Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add node logger code generation #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Torakushi
Copy link

@Torakushi Torakushi commented Sep 26, 2023

In breez/breez-sdk-greenlight#467, we give the opportunity to provide a logger and an optional log file path, when connecting to a node.

this PR reflects these changes when generating RN bindings

@dangeross
Copy link
Contributor

dangeross commented Sep 26, 2023

Looking good @Torakushi. I think it works well for the RN framework as its written currently.

There is a bigger rework that needs to be made for the React Native library to really take advantage of this, which is to make it multi-instance compatible. The typescript client and native code have to manage a Breez SDK instance over the RN bridge, which probably means communicating with an additional instance ID and emitting from the native code to a specific instance handler.

@Torakushi
Copy link
Author

Torakushi commented Sep 27, 2023

Right, I was thinking about tha, the multi instance PR will be quite more complex indeed (unless you meant that it has to be done right know?)

Anyway, here are some solutions to dig maybe to make the RN app multi-instances friendly:

  • make "supportedEvent" dynamic, each time we call connect, we will add a new event in the supportedEvent. (But quite unsure that we can do that way? if we can, it will be easier!)

  • If we can't make supportedEvent dynamic, we create a BreezSDKLogListenerDispatcher, that will listen to breezSdkNodeLog and then dispatch it to the right node_logger using an ID like you suggested for example.

@roeierez
Copy link
Member

Right, I was thinking about tha, the multi instance PR will be quite more complex indeed (unless you meant that it has to be done right know?)

@Torakushi these are good ideas that Ross and you are adding but certainly not part of this PR.

@dangeross
Copy link
Contributor

Happy to keep the scope for this PR minimal to accompany your other PR. I don't think there is a huge use-case for multi-instances in RN, so we can think about it more broadly in a follow-up PR.

@Torakushi Torakushi marked this pull request as ready for review September 29, 2023 07:34
@Torakushi Torakushi changed the title WIP: add node logger code generation add node logger code generation Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants